-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cpu/{atmega_common,atxmega}: increase idle thread stack size #18263
Conversation
cpu/atxmega/include/cpu_conf.h
Outdated
#else | ||
#define THREAD_STACKSIZE_IDLE (192) | ||
#define THREAD_STACKSIZE_IDLE (128) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, 192 -> 128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-pasted error :/ Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does ATXMega need more stack @nandojve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To some degree this can be explained with larger AVRs the RAMPZ, RAMPY, RAMPX, RAMPD and the 3 byte program counter, so that 5 bytes more are pushed onto the stack in case of context switching or IRQs. But the diff is quite a bit more than 5 bytes.
(Note that these registers are also not unique to ATXMegas, the better ATMegas can also have them.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benpicco , @maribu is correct! It is important mention that call on large devices have a 24-bit PC. So, depending how code is generated it may be good have some additional stack space. This is not restricted to ATXmega it is about devices that have EIND.
(Rare) models with >128 KiB of ROM have a 3-byte program counter. Subroutine calls and returns use an additional byte of stack space, there is a new EIND register to provide additional high bits for indirect jumps and calls, and there are new extended instructions EIJMP and EICALL which use EIND:Z as the destination address. (The previous IJMP and ICALL instructions use zero-extended Z.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course, the add additional byte for $pc
is not only needed in the context switch but for every function call :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still does this require to double the required stack size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can later run a test to measure what exact size it needs. I also intend to merge this to a common header in avr8_common
, which takes __AVR_3_BYTE_PC__
, __AVR_HAVE_RAMPZ__
, __AVR_HAVE_RAMPY__
, __AVR_HAVE_RAMPX__
, and __AVR_HAVE_RAMPD__
into account for the stack size, as some ATmega boards also have large stack requirements as well.
Our AVR port doesn't make use of an ISR stack and just victimizes the stack of whatever thread happens to be running, which in most cases is the idle thread. Hence, the idle stack has to be large enough to support the ztimer ISR.
1a3a31f
to
8cc0199
Compare
Hmpf, the unrelated hash mismatches are still a thing |
Contribution description
Our AVR port doesn't make use of an ISR stack and just victimizes the
stack of whatever thread happens to be running, which in most cases is
the idle thread. Hence, the idle stack has to be large enough to
support the ztimer ISR.
Testing procedure
Run program given in #18245 (comment) - with
master
you'll get something likebut with this PR the stack overflow should no longer be detected. (Test successfully with both Alpine's AVR toolchain in
BUILD_IN_DOCKER=1
.)Issues/PRs references
Reported in #18245 (comment)